Closed
Bug 771273
Opened 12 years ago
Closed 12 years ago
Make nsDocShell::GetSameTypeParent and friends respect <iframe mozbrowser> boundaries
Categories
(Core :: DOM: Navigation, defect)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: justin.lebar+bug, Unassigned)
References
Details
Attachments
(5 files, 1 obsolete file)
1.42 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
5.26 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
3.37 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
7.52 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
1.85 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
In-process <iframe mozbrowser> is being used more and more, in places I didn't expect. See for example bug 762993. If we're loading arbitrary code inside in-process mozbrowser, we need to lock it down properly. First step is window.open(_top/parent).
Reporter | ||
Updated•12 years ago
|
Blocks: browser-api
Reporter | ||
Comment 1•12 years ago
|
||
Actually, now that I've thought about this more, I'm starting to think we should just fix nsDocShell::GetSameTypeParent and nsDocShell::GetSameTypeRoot to respect <iframe mozbrowser> boundaries. That's much better than playing whack-a-mole, as I've done with the other in-process bugs. I'd bee hoping not to do this, but perhaps the time has come. I guess the good news is, we can be aggressive and switch almost everything to respecting mozbrowser boundaries. If things break, it'll only be where mozbrowser is used, and the change will make us much less broken than we currently are!
Reporter | ||
Updated•12 years ago
|
Component: DOM: Mozilla Extensions → Document Navigation
Summary: BrowserAPI: Handle _top and _parent targets correctly in <iframe mozbrowser> → Make nsDocShell::GetSameTypeParent and friends respect <iframe mozbrowser> boundaries
Reporter | ||
Comment 2•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=f05856f40d6a
Reporter | ||
Comment 3•12 years ago
|
||
I'm not sure that this is entirely correct -- there's a lot of code to audit, and I'm not an expert -- but it's unlikely to be worse than what we currently have. nsGlobalWindow basically ignores this change and keeps doing what it has done. That may not be entirely correct, but it's definitely necessary in some places.
Attachment #639924 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 4•12 years ago
|
||
Attachment #639925 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 5•12 years ago
|
||
There are a lot more things we could test here -- without looking at the code, _parent and window.content come to mind. But this is a start...
Attachment #639926 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 6•12 years ago
|
||
Attachment #639927 -
Flags: review?(bzbarsky)
Comment 7•12 years ago
|
||
Comment on attachment 639924 [details] [diff] [review] Part 1: Make nsDocShell::GetSameTypeParent and friends respect <iframe mozbrowser> boundaries. >+++ b/docshell/base/nsDocShell.cpp > nsDocShell::GetSameTypeParent(nsIDocShellTreeItem ** aParent) Can you make some of the nsGlobalWindow stuff call this now? >+nsDocShell::GetParentIgnoreBrowserFrame(nsIDocShell** aParent) This doesn't make it clear that the parent needs to be same-type... I'm not sure how to indicate that without a ridiculous method name. :( > nsDocShell::IsFrame() >+ return !mIsBrowserFrame && parent; If mIsBrowserFrame, then parent will be null, no? So can't this just |return parent;|? >+++ b/dom/base/nsGlobalWindow.cpp > nsGlobalWindow::GetContent(nsIDOMWindow** aContent) >- // If we're called by non-chrome code, make sure we don't return >- // the primary content window if the calling tab is hidden. In >- // such a case we return the same-type root in the hidden tab, >- // which is "good enough", for now. .... >+ // If we're called by non-chrome code, this is the same as window.top(). That's not what the old code did. In particular, window.content in a non-chrome sidebar was decidedly not the same as window.top... Not that we have test coverage for that gunk. :( r- until we get this sorted out. > nsGlobalWindow::GetRealFrameElement(nsIDOMElement** aFrameElement) > // We're at a chrome boundary, don't expose the chrome iframe > // element to content code. See, I'm not clear why the same considerations don't apply to mozbrowser... but anyway. This is certainly at least preserving behavior.
Attachment #639924 -
Flags: review?(bzbarsky) → review-
Comment 8•12 years ago
|
||
Comment on attachment 639925 [details] [diff] [review] Part 2: Make in-process <iframe mozbrowser>'s have their own shistory objects. r=me
Attachment #639925 -
Flags: review?(bzbarsky) → review+
Comment 9•12 years ago
|
||
Comment on attachment 639926 [details] [diff] [review] Part 3: Test that window.open(..., 'top') works properly for in-process <iframe mozbrowser>. r=me
Attachment #639926 -
Flags: review?(bzbarsky) → review+
Comment 10•12 years ago
|
||
Comment on attachment 639927 [details] [diff] [review] Part 4: Enable BackForward test for in-process <iframe mozbrowser>, now that it works. r=me
Attachment #639927 -
Flags: review?(bzbarsky) → review+
Reporter | ||
Comment 11•12 years ago
|
||
> See, I'm not clear why the same considerations don't apply to mozbrowser... What considerations, exactly? You mean, why does GetRealTop exist? I haven't looked too hard, but there's at least some widget-y code which needs to find chrome windows. > nsDocShell::GetSameTypeParent(nsIDocShellTreeItem ** aParent) > > Can you make some of the nsGlobalWindow stuff call this now? You mean, like nsGlobalWindow::GetScriptableParent? It's kind of awkward; what we really need is nsIDocShell::GetSameTypeParent2 which does /not/ respect mozbrowser boundaries, so we can implement GetRealParent in terms of that. But I don't think GetScriptableParent2 has a lot of other potential uses. >>+nsDocShell::GetParentIgnoreBrowserFrame(nsIDocShell** aParent) > > This doesn't make it clear that the parent needs to be same-type... I'm not sure how to indicate > that without a ridiculous method name. :( My thoughts exactly. I'll try to think up a better name, but I'm not hopeful.
Reporter | ||
Comment 12•12 years ago
|
||
Okay, I /think/ I see... If the window is "invisible" (background tab?), then we /do/ return window.top for the sidebar, right? That's totally bizarre. Can I tell whether the window is a sidebar? I'm not having good luck searching for that.
Comment 13•12 years ago
|
||
> You mean, why does GetRealTop exist? No, why GetRealChromeElement exists. > If the window is "invisible" (background tab?), then we /do/ return window.top for the > sidebar Well, the sidebar is never really invisible... Why not just keep the existing visibility check?
Reporter | ||
Comment 14•12 years ago
|
||
> No, why GetRealChromeElement exists. Oh, GetRealFrameElement. Yes, we can nix that. Cool. > Well, the sidebar is never really invisible... I give up. What's a sidebar? > Why not just keep the existing visibility check? AIUI, the existing code says: 1) If we're invisible, window.content returns SameTypeRootTreeItem (which respects mozbrowser) 2) Otherwise, window.content returns the tree owner's primary content shell (which doesn't respect mozbrowser). The problem with keeping the visibility check is that, if we're visible but not a sidebar, we don't want to return the primary content shell. I think we want to change the predicate in (1) to "If we're invisible or not a sidebar", but I don't know how to do that. Or maybe I'm missing something here, because I really have no idea what this sidebar business is about.
Comment 15•12 years ago
|
||
> I give up. What's a sidebar?
In Firefox, try View > Sidebar > Bookmarks?
But there are extensions that put other things in there too.
I see now what your problem is with the code as is is. What's the equivalent of "primary content shell" in the mozbrowser world, assuming there is one?
Reporter | ||
Comment 16•12 years ago
|
||
> What's the equivalent of "primary content shell" in the mozbrowser world, assuming there is one?
I guess it depends on your perspective. If you're inside a mozbrowser, the primary content shell should be the same as window.top, right? You can't see outside the mozbrowser. If you're outside mozbrowser, then the primary content shell is the same as it currently is.
I guess that answers my question about how to implement this, doesn't it?
Comment 17•12 years ago
|
||
Sounds plausible. ;)
Reporter | ||
Comment 18•12 years ago
|
||
>> No, why GetRealChromeElement exists.
>
> Oh, GetRealFrameElement. Yes, we can nix that. Cool.
If it's OK with you, I'd like to handle this in a follow-up. I have enough trouble sticking patches in the tree these days, and I don't need to further complicate this landing...
Reporter | ||
Comment 19•12 years ago
|
||
Attachment #641904 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 20•12 years ago
|
||
Comment on attachment 641904 [details] [diff] [review] Part 1, v2: Make nsDocShell::GetSameTypeParent and friends respect <iframe mozbrowser> boundaries. Interdiff part 1 --> part2: is $ interdiff <(curl -L https://bug771273.bugzilla.mozilla.org/attachment.cgi?id=641904) <(curl -L https://bug771273.bugzilla.mozilla.org/attachment.cgi?id=639924)
Reporter | ||
Updated•12 years ago
|
Attachment #639924 -
Attachment is obsolete: true
Reporter | ||
Comment 21•12 years ago
|
||
Attachment #641906 -
Flags: review?(bzbarsky)
Comment 22•12 years ago
|
||
> If it's OK with you, I'd like to handle this in a follow-up.
Worksforme.
Comment 23•12 years ago
|
||
Comment on attachment 641904 [details] [diff] [review] Part 1, v2: Make nsDocShell::GetSameTypeParent and friends respect <iframe mozbrowser> boundaries. r=me
Attachment #641904 -
Flags: review?(bzbarsky) → review+
Comment 24•12 years ago
|
||
Comment on attachment 641906 [details] [diff] [review] Part 5: Add test for window.content inside <iframe mozbrowser>. r=me
Attachment #641906 -
Flags: review?(bzbarsky) → review+
Comment 25•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3bfdf04d02f4 https://hg.mozilla.org/mozilla-central/rev/307af50437ef https://hg.mozilla.org/mozilla-central/rev/eb1e4ede61f1 https://hg.mozilla.org/mozilla-central/rev/ed117c0cde36 https://hg.mozilla.org/mozilla-central/rev/edc0540d1c3e
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•